Seems strange to export this - it doesn't really do anything by itself, it just writes a single field. I think the more useful things to export are encode_tlv_stream and decode_tlv_stream. That would require exporting this, but I think we should #[doc(hidden)] this and prefix it with a _ to indicate that it isn't, itself, a stable API or something users should use directly.

Yes sorry this needs to be re-worked.

I removed the serialization commit for now, need more time to work on that.

A message shouldn't have a default type, I think Custom needs to store the type somehow.

Agreed. I think we want T: Encode. Then implementors would typically use an enum for T as the the custom message type and each variant would wrap a struct that also implements Encode, just like wire::Message.

@jkczyz how do you implement Encode for an enum with the const requirement?

Hmm... good point. I guess it could instead be Custom(MessageType, T) or implement Encode for the enum as 0 and override type_id.

With Custom(MessageType, T) if I'm not mistaken it would only allow you to have a single value for MessageType so I don't think it's great. The override would probably work but doesn't feel super clean, and it would make the API a bit complicated on the user side I feel.

Here T is the enum, right? If so, it could take on an variant.

I still don't really see what you mean tbh but since it's not relevant with the new approach maybe no need to spend more time on that.

So it's back to being relevant and I think I got it (I hope at least).

Why store a T in Custom? Can't we get away with just making Custom store a Vec<u8> instead, avoiding a ton of generic typing for a similar outcome?

See background discussion here: https://lightningdevkit.slack.com/archives/C01AWTU1QAF/p1627528316011300?thread_ts=1627456353.011200&cid=C01AWTU1QAF

This is a bit more elegant to a previous approach and reduces the amount of code that needs to change in peer handler.

reduces the amount of code that needs to change in peer handler

I'm pretty dubious of this - there's a lot of new generic types flying everywhere and we already need to call a new method in the decode pipeline anyway. Maybe more importantly, the new API isn't trivially composable. Imagine I want to have a DLCMessageHandler and a MattsToSMessageHandler attached at once - I can't just match the message type and pass the message onwards to the next handler, I have to create a whole new enum that maps to multiple different potential message enums, which blows up the complexity substantially.

I'm pretty dubious of this - there's a lot of new generic types flying everywhere and we already need to call a new method in the decode pipeline anyway.

The proposed changes to peer_handler seemed quite extensive comparatively: p2pderivatives#3.

The approach in this PR is an alternative that I thought was much simpler. Additionally, separating parsing from handling seemed a cleaner separation of abstractions similar to wire and ChannelMessageHandler.

Happy to consider other alternatives.

Maybe more importantly, the new API isn't trivially composable. Imagine I want to have a DLCMessageHandler and a MattsToSMessageHandler attached at once - I can't just match the message type and pass the message onwards to the next handler, I have to create a whole new enum that maps to multiple different potential message enums, which blows up the complexity substantially.

We should be able to implement the trait on a 2-tuple like we do for chain::Listen. It would require defining an enum with two variants to use for the return value. The implementation of read would cascade as needed. And by nesting tuples it could be arbitrarily composable.

I suppose we could also use a trait object for the return type if you want to reduce the number of generic types. Figured we wanted to avoid dyn however.

W.r.t to composability I did have in mind to push the conversation behind the UMH interface. One implementation could be a proxy chaining processing calls on a vec of dynamic objects ?

I think most users won't set an unknown message handler, so this should instead default to IgnoringUnknownMessageHandler and not get the new templates.

Done

I don't get why we need the T here. Even if we keep the generic type for unknown messages, I think it should be an associated type on the UnknownMessageReader.

See background discussion here: https://lightningdevkit.slack.com/archives/C01AWTU1QAF/p1627528316011300?thread_ts=1627456353.011200&cid=C01AWTU1QAF

This is a bit more elegant to a previous approach and reduces the amount of code that needs to change in peer handler.

Agreed. I think we want T: Encode. Then implementors would typically use an enum for T as the the custom message type and each variant would wrap a struct that also implements Encode, just like wire::Message.

For consistency, could you name this CustomMessageHandler?

Done

My preference would be to remove the circular dependency between peer_handler and wire by defining this here as Read perhaps, so it can be used elsewhere as wire::Read.

Done (note that then it requires making the wire module public but I think it's fine?)

Is Debug needed?

nit: I would say "unrelated to the channel/gossip layers" as those custom/unknown messages are still defined in the terms of BOLT 1 and 8.

Used your phrasing in new version

I think leftover from downstream.

Maybe you could pass a predicate to this new method to evaluate if the peer not only identify to the given public key but also announce the feature signaling support for the relayed messages ?

IIUC this is just to check that the init message was received before we try to send something. I kind of see what you mean I think but I feel better to keep it simple for now and maybe add this kind of possibilities after.

W.r.t to composability I did have in mind to push the conversation behind the UMH interface. One implementation could be a proxy chaining processing calls on a vec of dynamic objects ?

Can we name this something more specific (eg CustomMessageReader)? Read conflicts with std::io::Read which is used pretty extensively.

Done

So I think Jeff was originally thinking this would be in PeerManager with the original read code from https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commitdiff;h=7a586455c10d835a425fa1de8ec447acbf9c67f3 there. That said, your approach here definitely cleans up the PeerManager message handling pipeline a lot. Still, the line count here is huge, I wonder if its easier to to just make wire::Message pub(crate) instead of pub, then just inline the Custom variant there instead of having a separate enum.

That way we'd avoid most of the diff, but still keep the read stuff nice and tight.

Thanks that works as well indeed, I had a misunderstood I guess.

I'm not a huge fan of this being defined in a different file from wire::Read, tbh. Maybe it makes the most sense to put both here?

Moved, but note that the reason for having them separated was to avoid circular dependency between wire and peer_handler as noted before (#1031 (comment)). Now it's back because CustomMessageHandler refers to MessageHandlingError which is in peer_handler.

Ah, I mean I don't care either way, then. Feels strange, but whatever.

I don't care either, @jkczyz do you prefer that I put it back in peer_handler to avoid the circular dependency?

I'd prefer to keep them separated, but note that they can be independent traits. No need to make one a super trait of the other since you already constrain the generic type by both traits.

I moved back the CustomMessageHandler trait into peer_handler.rs to remove the circular dependency. However I didn't manage to make them independent traits. The issue AFAIU is that the compiler needs to know that both traits' associated type are the same in order to be able to use the output of read as an input to handle_custom_message. Maybe it's possible to put this constraint somewhere else? (Although I must say even if it's possible I personally find it cleaner to have the constraint directly on the traits since I can't see a case where you'd want the types to differ).

I don't know if this log message is important or not (looks useful but not critical to me). I couldn't find a nice way to keep it without putting a Debug constraint on the T in Message (which is maybe also fine).

Ah, I didn't realize that is why it was needed. I think we want the constraint on T then.

Added and restored the log message

How about CustomMessage? Type is redundant and could be confused with the MessageType struct.

Agreed, updated

We haven't been consistent, but when breaking long lines could you use these conventions?

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/items.md#where-clauses

Done (also improved indents in other places following these guidelines)

This doesn't need to be a subtrait since peer_handler uses both as a trait bound.

See #1031 (comment)

Could you break this line up and possibly use a where clause for readability?

Broke up the line, but couldn't use a where clause because it's not a trait AFAIU. I also tried to use a type alias but it seems like type aliases don't support associated types for now.

Ah, I didn't realize that is why it was needed. I think we want the constraint on T then.

Actually, on second thought, can we not expose this and just have CustomMessageHandlers return LightningErrors? LightningError should cover all possible cases that users will want to trigger, PeerHandleError is somewhat more of a thing that we return to users from PeerManager, making both be used for receiving errors from users and returning errors to use blurs them a bit much IMO.

Works for me, done.

This seems really hard to use correctly - right now if you receive a message via handle_custom_message and are in the midst of processing it, you cannot call enqueue_custom_message or you'll deadlock. This seems like precisely the way users would implement this API, though. I think the only other way you can reasonably go about this would be to add the ability to get the message id from the CustomMessage associated type (which is already going to be an enum.

Oh that's a really good point.

Though I must say I don't understand what you would like to do instead. I tried several things previously including reusing the associated type for sending, but IIRC the main issue is the const requirement on the Encode trait, which makes it impossible to implement for an enum (or anything that has more than one type id).
I had also tried some complicated things like passing callbacks around but it always ended up requirement some boxing which didn't feel really nice.

For now I added a (mutexed) vector of node id + encoded messages on the peer handler, that can be used by the custom handler through the enqueue_custom_message method. It's a tiny bit a shame to have this while it might be mostly unneeded, but it felt like the simplest solution. Let me know what you think.

tried several things previously including reusing the associated type for sending, but IIRC the main issue is the const requirement on the Encode trait, which makes it impossible to implement for an enum (or anything that has more than one type id).

Yep, I think the only practical solution here is to create a new trait for public consumption that doesn't use the const. I suppose we could also drop the const in Encode and swap it for just a method, but that's a lot more diff and I dunno if its worth it.

I added the TypedMessage trait and made a blanket implementation for T: Encode so that the write function can be used by both. Because of that Encode needs to be public but apart from that I think that's a good way to reduce the diff.

I also updated the CustomMessageHandler to have a get_and_clear_pending_msg method so that it's more consistent with other handlers.

Does it make sense to use the MessageType newtype here (and basically in all the new code with message types) instead of u16?

You mean the MessageType(u16)? I tried at some point but it would require making the u16 public, and then from a user perspective you'd have to wrap your u16 into MessageType and unwrap them when matching, so I didn't feel like it provided big benefit.
If I'm being honest even within LDK I didn't feel like it provided a lot apart from the is_even method, but maybe I'm missing something. It would be nice if it could somehow abstract the underlying type but since it needs to be serialized as a u16 at the end I'm not sure it's possible.

Also even with the nice dummy wrapper module it doesn't seem to be possible to make MessgeType non public.

I don't feel strongly, tbh, it just felt awkward to have a newtype and not use it, maybe @jkczyz feels strongly.

Sorry I fell off the review. I think we could get rid of MessageType and clean up Encode by moving type_id to the new trait. Do you mind taking the commits in https://github.com/jkczyz/rust-lightning/tree/2011-08-wire-type? I did this there along with some other clean-ups with naming. It moves is_even to Message, which can be used on the Custom variant.

I cherry picked your commits. It makes things really clean IMHO, thanks!

Note that you can avoid making Encode public with a dummy wrapper module - the following compiles (but obviously use a better module name and fix the indentation).

@@ -251,2 +251,4 @@ pub fn write<M: TypedMessage + Writeable, W: Writer>(message: &M, buffer: &mut W
 
+pub(crate) mod lulz {
+use super::*;
 /// Defines a type-identified encoding for sending messages over the wire.
@@ -264,2 +266,4 @@ pub trait Encode {
 }
+}
+pub(crate) use self::lulz::Encode;
 

That's a nice trick thanks!